Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added Trimmomatic, support contig for bining, set concoct default parameters #745

Open
wants to merge 26 commits into
base: dev
Choose a base branch
from
Open

Added Trimmomatic, support contig for bining, set concoct default parameters #745

wants to merge 26 commits into from
+527 −6

Conversation

Pranjal-Bioinfo
Copy link

@Pranjal-Bioinfo Pranjal-Bioinfo commented Jan 17, 2025 •
edited
Loading

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/mag branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@Pranjal-Bioinfo Pranjal-Bioinfo changed the base branch from master to dev January 17, 2025 09:03
sateeshperi
sateeshperi requested changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@sateeshperi sateeshperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to make the use of scaffolds/contigs for binning a param rather than a replacement

Pranjal-Bioinfo and others added 23 commits January 20, 2025 20:07
@Pranjal-Bioinfo @GallVp
Update modules.config
dd13493 
Included Trimmomatic
@Pranjal-Bioinfo @GallVp
Update test.config
2effc94 
Added clip tool as trimmmomatic.
removed skip_concoct=true
@Pranjal-Bioinfo @GallVp
Update modules.json
aa94221
@Pranjal-Bioinfo @GallVp
Create Trimmomatic
94e9f54
@Pranjal-Bioinfo @GallVp
Delete modules/nf-core/Trimmomatic
088207e
@Pranjal-Bioinfo @GallVp
Create environment.yml
f5effeb
@Pranjal-Bioinfo @GallVp
Create main.nf
c4bf5e4
@Pranjal-Bioinfo @GallVp
Create meta.yml
42fef90
@Pranjal-Bioinfo @GallVp
Create main.nf.test
42f9906
@Pranjal-Bioinfo @GallVp
Create main.nf.test.snap
4da22b6
@Pranjal-Bioinfo @GallVp
Create nextflow_PE.config
574ff5c
@Pranjal-Bioinfo @GallVp
Create nextflow_SE.config
9762659
@Pranjal-Bioinfo @GallVp
Create tags.yml
72c03d5
@Pranjal-Bioinfo @GallVp
Update nextflow_schema.json
2327da5
@Pranjal-Bioinfo @GallVp
Update shortread_preprocessing.nf
911da74
@Pranjal-Bioinfo @GallVp
Update shortread_preprocessing.nf
0f324b7
@Pranjal-Bioinfo @GallVp
Update shortread_preprocessing.nf
68b1383
@Pranjal-Bioinfo @GallVp
Update mag.nf
881f554
@Pranjal-Bioinfo @GallVp
Update modules.config
af5adac 
added ext.args ='c 10000  -o 0 --merge_last at 642
@GallVp
Renamed folder from Trimmomatic to trimmomatic
b8f944d
@GallVp
Fixed linting issues
af8b495
@GallVp
Removed test config change
84e7186
@GallVp
Added parameter spades_use_contigs_not_scaffolds
b1eb9ed
@GallVp GallVp force-pushed the Trimmomatic-contig-concoct branch from 57f463f to b1eb9ed Compare January 20, 2025 07:30
@GallVp
Copy link
Member

GallVp commented Jan 20, 2025

need to make the use of scaffolds/contigs for binning a param rather than a replacement

I have added a new parameter spades_use_contigs_not_scaffolds, rebased the branch on top of the current dev and restored logos to pass the lint checks. I can see that CHANGELOG.md has not been updated and there isn't any issue linked to this PR. Anyone of us taking care of that?

@sateeshperi
Copy link
Contributor

closes #744

@sateeshperi sateeshperi linked an issue Jan 20, 2025 that may be closed by this pull request
Add new features and Improvement to nf-core/mag: Trimmomatic, contigs param for binning, new concoct default params #744
Open
sateeshperi
sateeshperi reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@sateeshperi sateeshperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now....lets get a review from #mag ppl

jfy133
jfy133 reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please add running trimmomatic to one of the test configs so we know it can execute at least once

And generally missing docs!

  • Listing in the README
  • Missing output.md

Otherwise code looks good! Just some consistency things

conf/modules.config Show resolved Hide resolved
nextflow.config
@@ -59,6 +59,7 @@ params {
// assembly options
coassemble_group = false
spades_options = null
spades_use_contigs_not_scaffolds = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
spades_use_contigs_not_scaffolds = false
spades_use_contigs_not_scaffolds = false

I think I would rather be more explicity: --spades_outputtype contigs with enum contigs or scaffolds (Scaffolds as default for backgfwards compatibility)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sateeshperi sateeshperi sateeshperi requested changes

@jfy133 jfy133 jfy133 left review comments

@GallVp GallVp GallVp left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@Pranjal-Bioinfo Pranjal-Bioinfo

Labels
ready to review
Projects
None yet
Milestone
No milestone
4 participants
@Pranjal-Bioinfo @GallVp @sateeshperi @jfy133